-
Notifications
You must be signed in to change notification settings - Fork 39
fix: fetch MLS public keys the same way as feature config [WPB-17161] #3991
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
…droid [WPB-11304] (#3935) Co-authored-by: yamilmedina <[email protected]>
Co-authored-by: Mohamad Jaara <[email protected]>
Co-authored-by: Mohamad Jaara <[email protected]>
This reverts commit 5c4e3d8.
(cherry picked from commit 542f53c)
…into fix/fetch-mls-public-keys-on-foreground # Conflicts: # kalium
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #3991 +/- ##
========================================
Coverage 47.61% 47.61%
========================================
Files 509 509
Lines 17812 17814 +2
Branches 2925 2925
========================================
+ Hits 8481 8483 +2
Misses 8449 8449
Partials 882 882
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Built wire-android-staging-compat-pr-3991.apk is available for download |
This PR is stale because it has been open 30 days with no activity. Please update it or close it in case is not relevant anymore. |
…ic-keys-on-foreground # Conflicts: # .github/workflows/build-prod-app.yml # app/src/main/kotlin/com/wire/android/di/accountScoped/DebugModule.kt # app/src/main/kotlin/com/wire/android/ui/authentication/devices/register/RegisterDeviceScreen.kt # app/src/main/kotlin/com/wire/android/ui/authentication/login/email/LoginEmailViewModel.kt # app/src/main/kotlin/com/wire/android/ui/debug/DebugDataOptions.kt # app/src/main/kotlin/com/wire/android/ui/debug/DebugDataOptionsViewModel.kt # app/src/main/kotlin/com/wire/android/ui/home/AppSyncViewModel.kt # app/src/main/kotlin/com/wire/android/ui/home/HomeState.kt # app/src/main/kotlin/com/wire/android/ui/home/HomeViewModel.kt # app/src/main/kotlin/com/wire/android/ui/home/conversations/ConversationScreen.kt # app/src/main/kotlin/com/wire/android/ui/home/conversations/media/ConversationMediaScreen.kt # app/src/main/kotlin/com/wire/android/ui/home/conversations/messages/ConversationMessagesViewModel.kt # app/src/main/kotlin/com/wire/android/ui/home/conversations/messages/item/MessageContentAndStatus.kt # app/src/main/kotlin/com/wire/android/ui/home/conversations/model/messagetypes/audio/AudioMessageType.kt # app/src/main/play/release-notes/en-US/default.txt # app/src/test/kotlin/com/wire/android/ui/authentication/login/email/LoginEmailViewModelTest.kt # app/src/test/kotlin/com/wire/android/ui/calling/SharedCallingViewModelTest.kt # app/src/test/kotlin/com/wire/android/ui/home/AppSyncViewModelTest.kt # app/src/test/kotlin/com/wire/android/ui/home/HomeViewModelTest.kt # app/src/test/kotlin/com/wire/android/ui/home/conversations/messages/ConversationMessagesViewModelTest.kt # app/src/test/kotlin/com/wire/android/ui/settings/debug/DebugDataOptionsViewModelTest.kt # build-logic/plugins/src/main/kotlin/AndroidCoordinates.kt # kalium
…ic-keys-on-foreground # Conflicts: # kalium
Built wire-android-staging-compat-pr-3991.apk is available for download |
Built wire-android-dev-debug-pr-3991.apk is available for download |
…ic-keys-on-foreground # Conflicts: # kalium
|
Built wire-android-staging-compat-pr-3991.apk is available for download |
Built wire-android-dev-debug-pr-3991.apk is available for download |
@@ -83,6 +85,7 @@ class AppSyncViewModel @Inject constructor( | |||
listOf( | |||
viewModelScope.launch(dispatcher.io()) { syncCertificateRevocationListUseCase() }, | |||
viewModelScope.launch(dispatcher.io()) { featureFlagsSyncWorker.execute() }, | |||
viewModelScope.launch(dispatcher.io()) { mlsPublicKeysSyncWorker.executeImmediately() }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am pretty sure this whole block of runSyncTasks()
here was just a quick hack and now it is getting expanded further.
App knows too much about this.
We should either:
A. Launch mlsPublicKeysSyncWorker
when UserSessionScope
starts, and have it wait for Sync to be LIVE before doing anything else. Once LIVE it will do its thing, be it on the background or not. And it can reschedule itself to repeat every 24h if still running.
B. Create a ForegroundWorker
of some sorts in Kalium, so the app would call it and Kalium would take over.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vitorhugods should I do it as part of this ticket or should we create a dedicated one to clear this up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation states that it should be called each time the user puts the app in the foreground, so running it each time the app becomes LIVE is not exactly the same.
And it actually waits until live when being executed, this logic is implemented already inside this worker, just like for feature flag sync worker and proteus sync worker.
About B idea, I don't know if I get it correctly but the only change would be that the app starts this single ForegroundWorker
instead of starting each one separately?
PR Submission Checklist for internal contributors
The PR Title
SQPIT-764
The PR Description
What's new in this PR?
Issues
Fetching MLS public keys should be treated the same way as feature-configs.
https://wearezeta.atlassian.net/wiki/spaces/ENGINEERIN/pages/160858877/Feature+flags+and+feature+configuration#Periodic-fetch-of-configuration
Solutions
Add fetching MLS public keys to the
AppSyncViewModel
. Update kalium version to the one with MLS public keys changes to be able to useMLSPublicKeysSyncWorker
(it executes fetch every 24h as well).Dependencies (Optional)
Needs releases with:
Testing
Test Coverage (Optional)
PR Post Submission Checklist for internal contributors (Optional)
PR Post Merge Checklist for internal contributors
References
feat(conversation-list): Sort conversations by most emojis in the title #SQPIT-764
.